feat(Chip) replace with Label #621

Dominik-Petrik · 2024-03-26T11:07:59Z

close #564

adamviktora

Looking great! 🔥 I like the idea of separating the fixer helper functions.

Couple of comments bellow, but mostly minor changes. The only blocking one is related to the Label / LabelGroup import - what if it is already imported, the user could end up with duplicate import specifier.

adamviktora · 2024-03-26T14:35:19Z

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

+  ];
+};
+
+const getJSXAttribute = (node: JSXElement, attributeName: string) => {


This can be replaced with the getAttribute helper.

adamviktora · 2024-03-26T14:36:50Z

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

+  const importedName = importSpecifier.imported.name;
+  const name = importedName.replace(oldName, newName);
+  const localName = importSpecifier.local.name;
+  const aliasText = importedName !== localName ? ` as ${localName}` : '';
+  const rename = `${name}${aliasText}`;


Seems like these lines can be deleted as the rename is not used.

Rename should be used instead of newName in the fixer, thanks for the catch.

I think you can keep the newName there as it can make things easier afterwards when renaming the JSXElement, so you don't have to check if an alias was used there. But it is up to you. Also the duplicate Label import might play a role, so it might end up being more complicated than I see it now.

adamviktora · 2024-03-26T14:37:58Z

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

+    return [];
+  }
+
+  const oldName = importToRename.imported.name;


This line can also be deleted.

adamviktora · 2024-03-26T14:40:40Z

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

+  const rename = `${name}${aliasText}`;
+  return [
+    fixer.replaceText(importSpecifier, newName),
+    fixer.replaceText(node.source, "'@patternfly/react-core'"),


Is the replacement of the node.source necessary?

adamviktora · 2024-03-26T14:44:27Z

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

+              message: `Chip has been deprecated. Running the fix flag will replace Chip and ChipGroup components with Label and LabelGroup components respectively.`,
+              fix(fixer) {
+                return [
+                  ...renameImportSpecifier(chipImport, node, fixer, 'Label'),


What if the user already has a Label or LabelGroup imported? We should cover that case too, so there is no name clashing of duplicate Label imports.

Nice catch!

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

adamviktora · 2024-03-26T15:13:24Z

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

+                              node.closingElement.name,
+                              'Label'
+                            ),
+                            ...moveBadgeAttributeToBody(node, fixer, context),


Is it possible that someone would use a Chip with Badge but without children (self-closing)? I am just wondering, whether this case should be covered:

<Chip onClick={onClick} badge={<Badge isRead={true}>7</Badge>} />

I guess probably not, or ... it would be too much work to handle it and it is not very likely.

adamviktora · 2024-03-26T15:23:22Z

...es/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chipReplaceWithLabelInput.tsx

@@ -0,0 +1,3 @@
+import { Chip } from '@patternfly/react-core/deprecated';
+
+export const ChipReplaceWithLabelInput = () => <Chip onClick=(handleClick) badge={badge}></Chip>;


I think this was meant to be onClick={handleClick}.
Also can we add some text as children of the Chip? (to better demonstrate the badge transformation)

wise-king-sullyman

Awesome work on this! A few small refactor nits and Adam's existing comments and I think this will be good!

packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts

- covers Label imports already used - covers multiple imports from deprecated package - modify nested JSXElements in one rule run

adamviktora · 2024-04-10T11:47:18Z

Did some changes to handle the situation with Label import already included. Also there was a bug when there were more components imported from the deprecated package, they were all converted to the react-core package.

Also I discovered that when there were more elements nested, like this:

<ChipGroup>
  <Chip>Some chip</Chip>
</ChipGroup>

it would only modify the parent element on the first rule run (it eventually runs the rule 10 times, fixing the nested elements, but it is problematic with tests, where it only runs once) - due to some ranges clashing probably.

Because of that and also to be able to properly remove the unused Chip and ChipGroup imports (otherwise after removing, we are not able to get the previous local name - alias of the import), it was necessary to apply all the fixes in one ImportDeclaration matcher.

The only disadvantage is the consumers won't get errors for every line where the actual JSXElement is used, but only for the import declaration.

wise-king-sullyman

🚀

Dominik-Petrik requested review from wise-king-sullyman, thatblindgeye and adamviktora March 26, 2024 11:09

adamviktora requested changes Mar 26, 2024

View reviewed changes

wise-king-sullyman reviewed Mar 26, 2024

View reviewed changes

Dominik-Petrik added 2 commits April 8, 2024 21:16

feat(Chip) replace with Label

250bd1d

bug fixes and refactoring

01ba164

Dominik-Petrik force-pushed the replace-chip branch from 1bd422d to 01ba164 Compare April 8, 2024 19:16

Dominik-Petrik requested review from adamviktora and wise-king-sullyman April 8, 2024 19:24

adamviktora added 3 commits April 10, 2024 13:26

feat(helpers): getImportDeclaration helper

c1d6722

feat(chipReplaceWithLabel): handle Label imports

25b0a5b

- covers Label imports already used - covers multiple imports from deprecated package - modify nested JSXElements in one rule run

test(chipReplaceWithLabel): add tests

c4b2ea1

adamviktora approved these changes Apr 10, 2024

View reviewed changes

wise-king-sullyman approved these changes Apr 23, 2024

View reviewed changes

wise-king-sullyman merged commit 44f8afb into patternfly:main Apr 23, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Chip) replace with Label #621

feat(Chip) replace with Label #621

Dominik-Petrik commented Mar 26, 2024

adamviktora left a comment

adamviktora Mar 26, 2024

adamviktora Mar 26, 2024

Dominik-Petrik Mar 27, 2024

adamviktora Mar 27, 2024 •

edited

Loading

adamviktora Mar 26, 2024

adamviktora Mar 26, 2024

adamviktora Mar 26, 2024

Dominik-Petrik Mar 27, 2024

adamviktora Mar 26, 2024

adamviktora Mar 26, 2024

wise-king-sullyman left a comment

adamviktora commented Apr 10, 2024

wise-king-sullyman left a comment

		@@ -0,0 +1,3 @@
		import { Chip } from '@patternfly/react-core/deprecated';

		export const ChipReplaceWithLabelInput = () => <Chip onClick=(handleClick) badge={badge}></Chip>;

feat(Chip) replace with Label #621

feat(Chip) replace with Label #621

Conversation

Dominik-Petrik commented Mar 26, 2024

adamviktora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamviktora Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wise-king-sullyman left a comment

Choose a reason for hiding this comment

adamviktora commented Apr 10, 2024

wise-king-sullyman left a comment

Choose a reason for hiding this comment

adamviktora Mar 27, 2024 •

edited

Loading